feat(cli): enforce vite-plus import lint rule#1408
feat(cli): enforce vite-plus import lint rule#1408Han5991 wants to merge 48 commits intovoidzero-dev:mainfrom
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
1fe3184 to
3a43e6d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eceea6afe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Awesome, the import rewrite logic can be completely replaced with the oxlint plugin. |
Makes sense. I focused this PR on the unsafe rewrite fix first, and I can move the remaining import rewrite logic into the oxlint plugin in a follow-up. |
camc314
left a comment
There was a problem hiding this comment.
This looks good! but we should definitely use oxlint's types rather than handrolling it, it duplicates code, and might cause issues in future!
b757aaf to
57897c7
Compare
|
The failure appears to come from a version skew in the global snap test. In this test, The local CLI generates a lint config that references That means oxlint sees the registry-installed This also explains why the other |
|
I think there are three possible ways to address this.
The failure is caused by mixing the locally built CLI under test with This avoids the version skew and keeps the test focused on the current build. A packed tarball seems better than
Another option is to make That would keep the generated app’s registry-installed dependencies intact, but it changes production resolution behavior. I would be careful with this because it may be harder to justify unless built-in Vite+ plugins are explicitly meant to be provided by the running CLI rather than by the project’s installed
We could also leave the test behavior as-is and consider the failure expected until the package containing This is the most conservative option, but it means the global snap test can fail whenever the local CLI starts generating config that depends on an unreleased |
7930f65 to
7ab5df7
Compare
359fd37 to
273e4fb
Compare
| return fs.readFileSync(a).equals(fs.readFileSync(b)); | ||
| } | ||
|
|
||
| function assertGlobalCliBinaryMatchesCheckout(binDir: string, casesDir: string): void { |
There was a problem hiding this comment.
@Han5991 Can we create a new GitHub Action to test the oxlint plugin logic? This way we won't need to test through snap test. Currently, because we have to test through snap test, the modification logic for snap test has become too complex.
There was a problem hiding this comment.
Yes, I think we can add a dedicated GitHub Action/job for the oxlint plugin integration test. That should be a better place to verify that oxlint can load vite-plus/oxlint-plugin through package exports and that the rule/fix works, instead of relying on snap-test output.
One note: the snap-test changes here were added because the current global snap-test environment can still execute against the previously installed global vite-plus package. In that setup, even if this PR adds the ./oxlint-plugin export, oxlint may resolve vite-plus/oxlint-plugin from the stale installed package instead of the checkout and fail to find the plugin. That is why I forced the global CLI JS entry and relinked installed checkout packages.
I can split this so the oxlint plugin behavior is covered by a focused CI job, and keep only the minimal snap-test fix needed to ensure global snapshots run against the current checkout.
d38d6fc to
b6ef1e7
Compare
|
Started splitting plugin verification into a dedicated CI workflow and hit a tradeoff:
Options:
I'd lean (1). Which do you prefer? |
…re/runtime versions
…iers and remove unused dashmap
- Restore SNAP_CASES_DIR env var that fixtures reference for shared helpers - Restore dashmap entry for rolldown_devtools in Cargo.lock
- Split `vp lint` commands in `snap-tests/lint-vite-plus-imports` to isolate issues in individual files. - Updated test output to reflect separate error counts for `index.ts` and `types.ts`. - Bumped several dependencies in `pnpm-lock.yaml` to latest compatible versions. - Added config flags to `snap-test.ts` for non-interactive runs.
CI rebuilds rolldown_devtools with dashmap as a transitive dependency and then fails the post-snap-test `git diff Cargo.lock` check. Keep the entry to match what cargo emits in CI.
The `linkCheckoutPackages: true` mechanism added in 7b14470 only triggers when the preceding command exits 0. pnpm v11 inserts a placeholder into `pnpm-workspace.yaml` and exits 1 with ERR_PNPM_IGNORED_BUILDS when a template pulls in native packages with build scripts (esbuild/sharp from the astro template). That made `vp install` fail and the symlink swap never ran, so `vp check --fix` resolved to the npm-registry vite-plus which lacks the new `./oxlint-plugin` export and crashed during plugin loading. Pass `--ignore-scripts` to `vp install` in the astro fixture so the build-script gate is bypassed without weakening the security model (safer than `--dangerously-allow-all-builds`). vp check only runs lint/format and doesn't need the native binaries built. Also drop the debugging-era pnpm config env vars accidentally landed in d80cc45c.
Reflect the lint plugin config (`jsPlugins` + `rules`) that `createDefaultVitePlusLintConfig` now writes into vite.config.ts when the `prefer-vite-plus-imports` rule is enabled.
CI checks out a pinned vite tree that still declares `tsdown: ^0.21.9` in vite/packages/create-vite and vite/packages/plugin-legacy, but our lockfile carried `^0.21.10` (picked up from a stale local vite tree during a prior non-frozen install). Frozen-lockfile install in CI then aborts with ERR_PNPM_OUTDATED_LOCKFILE before any build/test runs. Roll the two specifier lines back to `^0.21.9` to match the checked-in vite tree without rewriting the resolved versions.
CI clones vite at .upstream-versions.json's pinned hash (32c2978), which still declares the older specifier set in vite/packages/vite/package.json. Our lockfile carried newer values picked up from a stale local vite checkout, so frozen-lockfile install failed with ERR_PNPM_OUTDATED_LOCKFILE on six packages (postcss, @vitejs/devtools, @vitest/utils, baseline-browser-mapping, periscopic, terser). Roll just the six specifier lines back to match main's lockfile, without touching the resolved versions or other importer entries.
The previous specifier-only fix wasn't enough — `@vitest/utils` is declared with an exact specifier, so its resolved version had to move back to 4.1.4 too, and the same applies to other transitive resolutions that drifted when our local vite checkout was newer than the pinned hash. Locally checked out vite at the pinned hash (.upstream-versions.json points to 32c2978 = v8.0.10) and ran `pnpm install --no-frozen-lockfile` so pnpm could re-resolve the affected entries cleanly. Verified with `pnpm install --frozen-lockfile` (Lockfile is up to date) before restoring the local vite working tree.
Avoid unnecessary config injection when `baseUrl` is already defined in the project's TypeScript configuration.
ea70476 to
f9c2371
Compare
bfa8c66 to
2b72c83
Compare
Update: musl workaround reapplied with root cause notesI tried removing the Alpine/musl skip workaround to see what was actually breaking and reproduce it locally, but the failure turned out to be a deeper native binding interaction — not a fixable scope for this PR. So I've reapplied the skip in What was actually failingIn the Alpine Root causeWhen Reproduced locally in
Why it's out of scope for this PRThis is a binary-level interaction between two unrelated NAPI modules (
Suggested follow-up (separate issue)
Happy to file the follow-up issue once this lands. |
Same musl NAPI conflict as lint-vite-plus-imports — `vp check --fix` SIGSEGVs when oxlint loads vite.config.ts with rolldown also in scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
vite/vitestimports tovite-pluslintconfigTesting
command-init-inline-configandlint-vite-plus-importsCloses #1301
Closes #1457